Ensure `reload` sets correct owner for each association (ActiveRecord::Persistence) #46383

dmytro-savochkin · 2022-10-29T23:48:31Z

Motivation / Background

Fixes #46363 which was caused by #41112

Currently calling reload re-calculates the association cache based on the object freshly loaded from the database. The fresh object gets assigned as owner for each association in the association cache. That object however is not the same as self (even though they both point to the same record). Under some specific circumstances (using has_one/has_many through associations and transactions with after_commit callbacks) this might lead to losing the effects of assignments done inside the callback.

This commit fixes it by making reload point to self as owner for each association in the association cache.

I think this is safe because as far as I can see in https://github.com/rails/rails/blob/main/activerecord/lib/active_record/associations.rb#L320 it always passes self when creating a new association.

Detail

This Pull Request changes ActiveRecord::Persistence#reload method.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
CI is passing.

dmytro-savochkin · 2022-10-30T00:05:41Z

@eileencodes, may I ask you to review please (since you were adding association cache line before)? Thanks!

nvasilevski · 2022-12-06T16:38:38Z

activerecord/lib/active_record/persistence.rb

@@ -971,6 +971,7 @@ def reload(options = nil)
      end

      @association_cache = fresh_object.instance_variable_get(:@association_cache)
+      @association_cache.each_value { |association| association.instance_variable_set(:@owner, self) }


I wonder what core team folks think on that but I always feel uncomfortable about using instance_variable_set unless we are patching some code we have no access to. Otherwise we are using something that wasn't supposed to be used even within the framework itself and makes me think whether we should add a public method to association to be able to set the owner. To be fair it might not be a solid argument as we already set and access instance variables on the lines above and below this one 🙃

Other than that, the PR makes sense to me.

Yes, this was exactly my thinking!

I am also not very comfortable using instance_variable_get/instance_variable_set but in this particular case I decided to go with it because:

This is already done one line above and below just as you said :)

I wasn't completely sure my approach would be accepted (dunno, maybe some other reasons I am not aware of) so doing some major change, like trying to introduce some new public method into association (which in turn might require changes to the docs, etc, etc) so that it can be used in this place, seemed a bit too premature to me.

But if you think it's worth it, let me know, and I will try to introduce my change via a new public method.
However the lines above and below will still be using instance_variable_get, so dunno... Changing those in this PR as well seems like a bit too much for such a small problem/PR.

I would say feel free to leave it as is for now, your reasoning makes total sense 👍 I guess I would have done the same to avoid prematurely overcomplicating the proposal 🙃

Yeah, let's avoid those instance variable set here. If we need to set the owner outside of that object, we should expose a writter.

No problem, updated.

dmytro-savochkin · 2022-12-22T20:38:24Z

Hey @nvasilevski!
Sorry for bothering but maybe you know who could be added to reviewers to this?
I understand sometimes it might take a few months here for a PR to be addressed but until I am completely sure it's the only option I'd like to try fasten things up a little :)

skipkayhil · 2022-12-22T20:44:45Z

Please don't ping individuals for review unless they have asked you to. The best way to get feedback would be asking in the Discord

dmytro-savochkin · 2022-12-22T21:14:01Z

Please don't ping individuals for review unless they have asked you to. The best way to get feedback would be asking in the Discord

Oh, wow, there is also a Discord! Thanks, I will try that.
So far however I've followed this guide for feedback and on the only thing mentioned there (ror-core mailing list) there was no reply.

dmytro-savochkin · 2023-10-12T20:10:09Z

This PR is still relevant for Rails 7.1.1, the issue is still there.

rafaelfranca · 2023-11-10T18:53:33Z

activerecord/test/cases/persistence/reload_association_cache_test.rb

+# The test case below is aimed to fix a bug that was noticeable only with a very specific
+# setup of models. The models are fully defined inside the test case so that they are not
+# accidentally changed later causing the bug effects to disappear.


Let's move this test to where it makes sense. You can define new models by creating new classes in the test/models folder, no need to do what is being done here.

Those models here still leaks to all the other files, so you aren't really isolating those models from other tests

Yep, updated as well.

rafaelfranca · 2023-11-10T18:56:42Z

activerecord/lib/active_record/persistence.rb

@@ -971,6 +971,7 @@ def reload(options = nil)
      end

      @association_cache = fresh_object.instance_variable_get(:@association_cache)
+      @association_cache.each_value { |association| association.instance_variable_set(:@owner, self) }


Yeah, let's avoid those instance variable set here. If we need to set the owner outside of that object, we should expose a writter.

Fix rails#46363. Currently calling `reload` re-calculates the association cache based on the object freshly loaded from the database. The fresh object gets assigned as `owner` for each association in the association cache. That object however is not the same as `self` (even though they both point to the same record). Under some specific circumstances (using `has_one/has_many through` associations and transactions with `after_commit` callbacks) this might lead to losing the effects of assignments done inside the callback. This commit fixes it by making `reload` point to `self` as `owner` for each association in the association cache.

…ad-association-cache Ensure `reload` sets correct owner for each association (ActiveRecord::Persistence)

rails-bot bot added the activerecord label Oct 29, 2022

nvasilevski reviewed Dec 6, 2022

View reviewed changes

dmytro-savochkin mentioned this pull request Feb 28, 2023

Loss of state after reload, transaction, and after_save_commit (regression in ActiveRecord 7.0) #46363

Closed

dmytro-savochkin force-pushed the fix-activerecord-reload-association-cache branch from 88d1737 to 70ef26f Compare October 12, 2023 19:48

zzak added this to the 7.1.2 milestone Oct 20, 2023

joshuay03 mentioned this pull request Oct 21, 2023

Add Discord invite link to contributing guide #49729

Merged

4 tasks

rafaelfranca reviewed Nov 10, 2023

View reviewed changes

rafaelfranca modified the milestones: 7.1.2, 7.1.3 Nov 10, 2023

dmytro-savochkin force-pushed the fix-activerecord-reload-association-cache branch from 70ef26f to 781ecc2 Compare November 18, 2023 14:09

dmytro-savochkin requested a review from rafaelfranca November 18, 2023 14:20

rafaelfranca merged commit af2bbd5 into rails:main Jan 15, 2024
4 checks passed

rafaelfranca added a commit that referenced this pull request Jan 15, 2024

Merge pull request #46383 from dmytro-savochkin/fix-activerecord-relo…

57a50f5

…ad-association-cache Ensure `reload` sets correct owner for each association (ActiveRecord::Persistence)

rafaelfranca added a commit that referenced this pull request Jan 15, 2024

Add changelog entry for #46383

21dff57

dmytro-savochkin deleted the fix-activerecord-reload-association-cache branch January 18, 2024 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure `reload` sets correct owner for each association (ActiveRecord::Persistence) #46383

Ensure `reload` sets correct owner for each association (ActiveRecord::Persistence) #46383

dmytro-savochkin commented Oct 29, 2022 •

edited

dmytro-savochkin commented Oct 30, 2022

nvasilevski Dec 6, 2022

dmytro-savochkin Dec 6, 2022

dmytro-savochkin Dec 6, 2022

nvasilevski Dec 7, 2022

rafaelfranca Nov 10, 2023

dmytro-savochkin Nov 18, 2023

dmytro-savochkin commented Dec 22, 2022

skipkayhil commented Dec 22, 2022

dmytro-savochkin commented Dec 22, 2022

dmytro-savochkin commented Oct 12, 2023

rafaelfranca Nov 10, 2023

dmytro-savochkin Nov 18, 2023

rafaelfranca Nov 10, 2023

Ensure reload sets correct owner for each association (ActiveRecord::Persistence) #46383

Ensure reload sets correct owner for each association (ActiveRecord::Persistence) #46383

Conversation

dmytro-savochkin commented Oct 29, 2022 • edited

Motivation / Background

Detail

Checklist

dmytro-savochkin commented Oct 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-savochkin commented Dec 22, 2022

skipkayhil commented Dec 22, 2022

dmytro-savochkin commented Dec 22, 2022

dmytro-savochkin commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ensure `reload` sets correct owner for each association (ActiveRecord::Persistence) #46383

Ensure `reload` sets correct owner for each association (ActiveRecord::Persistence) #46383

dmytro-savochkin commented Oct 29, 2022 •

edited